Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove public gtest dependency from libcudf conda package #15534

Merged
merged 9 commits into from
Apr 23, 2024
Merged

Remove public gtest dependency from libcudf conda package #15534

merged 9 commits into from
Apr 23, 2024

Conversation

robertmaynard
Copy link
Contributor

@robertmaynard robertmaynard commented Apr 15, 2024 •
edited by vyasr
Loading

Description

Reworks the cudftestutil and dependency chain to remove the public gtest dependency in libcudf conda package.
The libcudftestutil was previously made static due to issues using a static system GTest that wasn't build with fPIC. Using a GTest from rapids-cmake which is built with fPIC enabled, removes this restriction and allows us to remove the public depedency.

Some notes:

  • We need to align all of RAPIDS C++ projects on static GTest from rapids-cmake
  • None of the compiled components / classes of libcudftestutils publically depend on GTest
  • Two of the libcudftestutils header only components bring include gtest. Since these headers aren't required to be used we are going to consider them optional.
  • Therefore using these optional libcudftestutils header will require downstream users to bring in GTest.

Fixes #13381
Contributes to rapidsai/build-planning#32

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@robertmaynard robertmaynard added feature request New feature or request non-breaking Non-breaking change labels Apr 15, 2024
@robertmaynard robertmaynard requested review from a team as code owners April 15, 2024 17:54
@github-actions github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue conda labels Apr 15, 2024
@robertmaynard robertmaynard mentioned this pull request Apr 15, 2024
Closed
@bdice
Copy link
Contributor

bdice commented Apr 16, 2024

I merged the upstream to get CI unblocked.

vyasr
vyasr requested changes Apr 16, 2024
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I think this change is already safe across the rest of RAPIDS (rapidsai/build-planning#32), I suspect we need to do a bit more work in cudf. Specifically, I think we have to address #13381. Currently libcudftestutil.a exports a public dependency on GTest. Granting that it is already a static library, that still means that consumers of it will see APIs that have use gtest. What happens if someone tries to link against libcudftestutil.a, but has compiled their code using a different version of gtest? Won't that result in incompatible binaries?

In practice the only use case I'm aware of is cuspatial, which uses rapids-cmake and we control, so it's probably not a serious issue, but unless I'm misunderstanding it is a real potential issue that is worth trying to resolve before merging this.

@robertmaynard
Copy link
Contributor Author

While I think this change is already safe across the rest of RAPIDS (rapidsai/build-planning#32), I suspect we need to do a bit more work in cudf. Specifically, I think we have to address #13381. Currently libcudftestutil.a exports a public dependency on GTest. Granting that it is already a static library, that still means that consumers of it will see APIs that have use gtest. What happens if someone tries to link against libcudftestutil.a, but has compiled their code using a different version of gtest? Won't that result in incompatible binaries?

In practice the only use case I'm aware of is cuspatial, which uses rapids-cmake and we control, so it's probably not a serious issue, but unless I'm misunderstanding it is a real potential issue that is worth trying to resolve before merging this.

This is a great point. Using a static GTest with libcudf would mean that conda consumers would be forced to never have GTest in the conda env. I agree with you that this is not a desired outcome, and it looks like we can't move over to a static GTest for libcudf without first splitting the testing component into a separate conda package.

I am going to close this PR in that case, as that is out of scope of this change

@robertmaynard robertmaynard reopened this Apr 18, 2024
@robertmaynard robertmaynard requested a review from a team as a code owner April 18, 2024 21:43
@robertmaynard
Copy link
Contributor Author

Re-opnening this PR since we realized that by always building our own GTest, we don't need to worry about linking to a static system GTest which wasn't built with fPIC.

@robertmaynard robertmaynard changed the title Use static gtest and gbench everywhere Remove public gtest dependency from libcudf conda package Apr 19, 2024
bdice
bdice approved these changes Apr 19, 2024
View reviewed changes
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed on request. Looks good.

vyasr
vyasr approved these changes Apr 19, 2024
View reviewed changes
cpp/tests/utilities/random_seed.cpp Show resolved Hide resolved
@vyasr
Copy link
Contributor

vyasr commented Apr 19, 2024

/merge

robertmaynard
robertmaynard commented Apr 23, 2024
View reviewed changes
cpp/include/cudf_test/cudf_gtest.hpp
@@ -16,86 +16,6 @@

#pragma once

#ifdef GTEST_INCLUDE_GTEST_GTEST_H_
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed now that we use a GTest version dowloaded via rapids-cmake. In addition the logic was incorrectly triggering on the devcontainer builds due to a system installed GTest causing us to break the internals of GTest

@rapids-bot rapids-bot bot merged commit 7341866 into rapidsai:branch-24.06 Apr 23, 2024
70 checks passed
@robertmaynard robertmaynard deleted the fea/static_test_dependencies branch April 23, 2024 20:06
@vyasr vyasr mentioned this pull request May 21, 2024
Closed
3 tasks
@robertmaynard robertmaynard mentioned this pull request Aug 21, 2024
Merged
3 tasks
@vyasr vyasr mentioned this pull request Aug 26, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr approved these changes

@bdice bdice bdice approved these changes

@ttnghia ttnghia ttnghia approved these changes

@AyodeAwe AyodeAwe AyodeAwe approved these changes

@shrshi shrshi Awaiting requested review from shrshi shrshi is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
Labels
CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
cuDF/Dask/Numba/UCX
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Remove libcudf dependency on gtest / gmock
5 participants
@robertmaynard @bdice @vyasr @ttnghia @AyodeAwe